Twisted Edwards curves operations #1949
Conversation
querolita
force-pushed
the
feature/eddsa/twisted
branch
from
b741643 to
576725d
Compare
| toBigint({ x, y }: Point) { | ||
| let x_ = Field3.toBigint(x); | ||
| let y_ = Field3.toBigint(y); | ||
| return { x: x_, y: y_, infinity: x_ === 0n && y_ === 1n }; |
There was a problem hiding this comment.
I created an equivalent Point type (elliptic-curve also has it) with the difference that the infinity is not always set to false as it is the case in the other gadgets.
| let witnesses = exists(12, () => { | ||
| let [x1_, x2_, y1_, y2_] = Field3.toBigints(x1, x2, y1, y2); | ||
|
|
||
| // TODO: reuse code in twistedAdd to avoid recomputing these |
There was a problem hiding this comment.
This would be a nice feature and much cleaner code, but apparently this wouldn't be a trivial refactor at all. Ideas for the future: inspiration from the interpreter structure in o1vm?
| * TODO: could use lookups for picking precomputed multiples, instead of O(2^c) provable switch | ||
| * TODO: custom bit representation for the scalar that avoids 0, to get rid of the degenerate addition case | ||
| */ | ||
| function multiScalarMul( |
There was a problem hiding this comment.
basically the difference between this function and the counterpart in elliptic-curve.ts is that the Curve is a CurveTwisted instead of an affine curve. I don't know if making this more generic would be a good idea, maybe we want to have a specific implementation for each curve instead of parameterizing it. Open to suggestions (similar concerns arise in other functions of this interface as well).
There was a problem hiding this comment.
Is the only difference between this function and the elliptic-curve.ts counterpart is that the Curve is a CurveTwisted? I'm curious to hear why you lean towards specific implementations of each curve vs a more generic function.
There was a problem hiding this comment.
If the curve is an affine curve there are some special cases that need to be treated which do not happen in the twisted case. That's the reason that I removed the zeros cases after receiving @mitschabaude's feedback here, so the type is not the only difference.
However, I also thought that having a more generic approach could be beneficial. But then I chatted a bit with @dannywillems, who believed that because elliptic curve operations are so sensitive, it can be a better idea to have almost duplicate implementations of certain functions, instead of maximizing generics. There's room for debate I suppose.
There was a problem hiding this comment.
Thanks for the explanation. I think it makes sense to keep it separate implementations for each curve.
|
Since twisted curve addition is complete, there's a lot of complexity in the scalar multiplication code around handling zeros that could be removed. E.g. you don't need the initial aggregator point, and you can just return the result instead of asserting it to be zero or non-zero |
|
@mitschabaude Oh that makes sense, I will look into it and clean the algorithm for twisted msm. |
There was a problem hiding this comment.
Given how complex the witness and assertion logic is for these curve functions, I'd feel more comfortable with more extensive testing.
e.g. instead of testing each method on a random point, can we test a few different points? Are there any special points that could be the source of an edge case error that we could test specifically? Can we also( or at least) test cases where the witnesses will be invalid and confirm that the assertions trigger?
There was a problem hiding this comment.
Was this file mistakenly added to git? There is no lib/ directory at the top level.
There was a problem hiding this comment.
oh this is probably a git error yeah, it should be in /src/lib/provable/test/twisted-curve.unit-test.js instead
There was a problem hiding this comment.
Looks like a compiled version of the unit test
querolita
force-pushed
the
feature/eddsa/twisted
branch
from
a0f99dc to
8e77d3a
Compare
This PR is the twisted counterpart of the operations found in
/src/lib/provable/gadgets/elliptic-curve.ts. It is a necessary step to support EdDSA, which uses the twisted curve Ed25519. Related to o1-labs/o1js-bindings#317.